Fix Python E2E test flakiness and missing pytest-timeout #391
Conversation
- Add ignore_errors=True to shutil.rmtree() in test cleanup to handle race conditions where files are still being written by background processes - Add missing_ok=True to unlink() for same reason - Add pytest-timeout to dev dependencies (fixes warning about unknown mark)
SteveSandersonMS
force-pushed
the
stevesa/python-e2e-fix
branch
from
7cac4d3 to
1e0b992
Compare
There was a problem hiding this comment.
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
python/e2e/testharness/context.py:112
- The approach of silently ignoring errors with
ignore_errors=Truecould mask legitimate issues beyond race conditions (e.g., permission problems, disk failures, or other filesystem errors). The Node.js SDK uses a more robust retry mechanism with exponential backoff (see nodejs/test/e2e/harness/sdkTestContext.ts:99-101) which would distinguish between transient race conditions and persistent failures.
Consider implementing a similar retry mechanism for Python instead of silently ignoring all errors. This would make cleanup more reliable while still providing visibility into persistent issues.
shutil.rmtree(item, ignore_errors=True)
Cross-SDK Consistency Review ✅I've reviewed this PR for consistency across all SDK implementations. No consistency issues found - this change maintains good cross-language parity. SummaryThis PR fixes Python E2E test flakiness by handling race conditions during test cleanup. All three major SDKs handle the same problem with language-appropriate patterns:
AnalysisThe Python change appropriately uses Python idioms ( The addition of Recommendation: ✅ Approve - maintains cross-SDK consistency while using appropriate language idioms.
|
1 participant
Summary
Fixes the Python E2E test failure in CI: https://github.com/github/copilot-sdk/actions/runs/21746470248/job/62733447842
Changes
1. Fix race condition in test cleanup (context.py)
The test fixture cleanup was failing with OSError: Directory not empty when shutil.rmtree() tried to delete session-state directories while background processes were still writing files.
Fix: Added ignore_errors=True to shutil.rmtree() and missing_ok=True to unlink() to gracefully handle these race conditions.
2. Add missing pytest-timeout dependency (pyproject.toml)
The test_compaction.py test uses @pytest.mark.timeout(120) but the pytest-timeout plugin was not installed, causing the warning: Unknown pytest.mark.timeout
Fix: Added pytest-timeout>=2.0.0 to dev dependencies.